Preserve insertion order of manually selected utxos if TxOrdering::Untouched
#262
+243
−236
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 15884240796Details
💛 - Coveralls |
nymius
force-pushed
the
fix/tx_build_should_not_change_order_of_insertion_with_vector
branch
from
7f16781 to
b1436ae
Compare
nymius
changed the title
nymius
changed the title
TxOrdering::Untouched
nymius
force-pushed
the
fix/tx_build_should_not_change_order_of_insertion_with_vector
branch
from
b1436ae to
dde0b3e
Compare
7 tasks
Since our logic is meant to enforce the precedence of local utxos, it makes sense to keep the test, but I agree there could be a simpler, easy to maintain version of it. ValuedMammal/bdk_wallet@49b5cef |
ValuedMammal
requested changes
Jun 24, 2025
nymius
force-pushed
the
fix/tx_build_should_not_change_order_of_insertion_with_vector
branch
from
dde0b3e to
048daa6
Compare
When TxBuilder::ordering is TxOrdering::Untouched, the insertion order of recipients and manually selected UTxOs should be preserved in transaction's output and input vectors respectively. Fixes bitcoindevkit#244
nymius
force-pushed
the
fix/tx_build_should_not_change_order_of_insertion_with_vector
branch
from
048daa6 to
d6d204b
Compare
|
Thanks for the review! |
7 tasks
Comment on lines
+438
to
+440
| if let Some(idx) = existing_index { | ||
| self.params.utxos.remove(idx); | ||
| } |
There was a problem hiding this comment.
question: why is it now being removed ?
Comment on lines
+2118
to
+2119
| // chain | ||
| // NOTE: this avoid UTxOs in both required and optional list |
There was a problem hiding this comment.
Suggested change
| // chain | |
| // NOTE: this avoid UTxOs in both required and optional list | |
| // chain | |
| // NOTE: this avoid UTXOs in both required and optional list |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
On my attempt to fix bitcoindevkit/bdk#1794 in bitcoindevkit/bdk#1798, I broke the assumption that insertion order is preserved when
TxBuilder::orderingisTxOrdering::Untouched. Some users are relying in this assumption, so here I'm trying to restore it back, without adding a new dependency for this single use case like #252, or creating a new struct just to track this.In this fourth alternative solution I'm going back to use
Vecto store the manually selected UTxOs.I was reluctant to do it in this way because
HashMapseems a better solution giving its property of avoiding duplicates, but as we also want to keep the secuential nature of the insertion of UTxOs inTxBuilder, here is an alternative aligned with that principle.May replace #252
May replace #261 .
Fixes #244
Notes to the reviewers
Also, as I was working on this, I came back to the following tests:
test_prexisting_foreign_utxo_have_no_precedence_over_local_utxo_with_same_outpointtest_prexisting_local_utxo_have_precedence_over_foreign_utxo_with_same_outpointMotivated during the implementation and review of bitcoindevkit/bdk#1798.
Which required the underlying structure to also hold the properties of no duplication for manually selected UTxOs, as the structures were accessed directly for these cases.
The test tries to cover the case when there are two wallets using the same descriptor, one tracks transactions and the other does not. The first passes UTxOs belonging to the second one and this one creates transactions using the
add_foreign_utxointerface.In this case it was expected for any
LocalUtxoof the offline wallet to supersede any conflicting foreign UTxO. But, the simulation of this case went against the borrowing constraints of rust.By how costly was to reproduce this behavior for me in the tests, I would like to have second opinions in the feasibility of the test case.
Changelog notice
No public APIs are changed by these commits.
Checklists
Important
This pull request DOES NOT break the existing API
cargo +nightly fmtandcargo clippybefore committing